Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MQE: fix issues with vector/vector binary comparsion operations #10235

Merged
merged 7 commits into from
Jan 10, 2025
Merged

MQE: fix issues with vector/vector binary comparsion operations #10235

merged 7 commits into from
Jan 10, 2025
+318 −197

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented Dec 13, 2024 •
edited
Loading

What this PR does

This PR fixes two issues with the implementation of comparison operations between two vectors without the bool modifier in MQE:

  • Queries would return incorrect results if the left side contained series with different metric names, and all results would be merged into a single series rather than returned in separate series for each metric name
  • Queries would incorrectly fail with a conflict error if multiple series on the left side matched the same series on the right side, those left side series had points at the same timestamp, and one or none of those points remained after applying the comparison operation

Changes in latency and peak memory consumption of affected benchmarks are within the margin of error of our benchmarks.

I'd recommend reviewing each commit separately.

Which issue(s) this PR fixes or relates to

#10067

Checklist

  • Tests updated.
  • [n/a] Documentation added.
  • [covered by Mimir Query Engine #10067] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • [n/a] about-versioning.md updated with experimental features.

@charleskorn
Add early filtering test case for group_right
45bb1c7
@charleskorn
Fix issue where comparison operations without the bool modifier would…
2d6b187 
… return incorrect results if the left side contained series with different metric names
@charleskorn
Avoid expensive labels.Labels.String() call
a2fef86
@charleskorn
Fix issue where comparison operations between two vectors incorrectly…
73b9e18 
… fail with a conflict if multiple left series match the same right series and only one left point remains after applying the comparison
@charleskorn charleskorn force-pushed the charleskorn/mqe-comparison-binop-fixes branch from d8b53a1 to 73b9e18 Compare January 8, 2025 04:27
@charleskorn charleskorn marked this pull request as ready for review January 10, 2025 03:22
@charleskorn charleskorn requested a review from a team as a code owner January 10, 2025 03:22
jhesketh
jhesketh approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@jhesketh jhesketh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but I am trying to think if we could add some extra tests around this and/or the annotation?

@charleskorn
Copy link
Contributor Author

lgtm, but I am trying to think if we could add some extra tests around this and/or the annotation?

Josh and I discussed this offline - this was a misunderstanding about some of the error messages.

@charleskorn charleskorn merged commit 56c9edd into main Jan 10, 2025
29 checks passed
@charleskorn charleskorn deleted the charleskorn/mqe-comparison-binop-fixes branch January 10, 2025 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhesketh jhesketh jhesketh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@charleskorn @jhesketh